Skip to content

bpo-30977: define uuid.UUID.__slots__ to save memory#2785

Closed
wbolster wants to merge 1 commit into
python:masterfrom
wbolster:uuid-slots
Closed

bpo-30977: define uuid.UUID.__slots__ to save memory#2785
wbolster wants to merge 1 commit into
python:masterfrom
wbolster:uuid-slots

Conversation

@wbolster

@wbolster wbolster commented Jul 20, 2017

Copy link
Copy Markdown
Contributor

@mention-bot

Copy link
Copy Markdown

@wbolster, thanks for your PR! By analyzing the history of the files in this pull request, we identified @serhiy-storchaka, @Yhg1s and @warsaw to be potential reviewers.

@wbolster wbolster changed the title bpo30977: define uuid.UUID.__slots__ to save memory 30977: define uuid.UUID.__slots__ to save memory Jul 20, 2017
@tiran tiran changed the title 30977: define uuid.UUID.__slots__ to save memory bpo-30977: define uuid.UUID.__slots__ to save memory Sep 7, 2017
@tiran tiran requested a review from warsaw September 15, 2017 11:19

@tiran tiran left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sensible to me. What do you think, @warsaw ?

@amannijhawan

Copy link
Copy Markdown

Will this get backported to 2.7.x?

@wbolster

Copy link
Copy Markdown
Contributor Author

hi, author here. is there anything i can do to help move this pr forward?

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change breaks pickle. See my comments on the tracker.

@bedevere-bot

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I didn't expect the Spanish Inquisition!. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but please add a test in test_exceptions() to make sure that is_safe attribute cannot be modified, near the "badtype(lambda: setattr(u, 'int', i))" test.

Comment thread Lib/uuid.py
self.__dict__['int'] = int
self.__dict__['is_safe'] = is_safe
super().__setattr__('int', int)
super().__setattr__('is_safe', is_safe)

@vstinner vstinner Sep 28, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to use object.__setattr__(self, 'int', int).

@vstinner

Copy link
Copy Markdown
Member

@serhiy-storchaka: "This change breaks pickle. See my comments on the tracker."

Oh, I missed this comment. It should be fixed.

Please rebase also your change.

@taleinat

taleinat commented Sep 6, 2018

Copy link
Copy Markdown
Contributor

Since this has lingered for a long time with no word from the original author, I've gone ahead and implemented appropriate pickle/unpickle logic. I've created a new PR (#9078) since this one is old and has merge conflicts.

I suggest closing this.

@vstinner vstinner closed this Sep 6, 2018
@wbolster

wbolster commented Sep 6, 2018

Copy link
Copy Markdown
Contributor Author

thanks all, especially @taleinat for finishing my first attempt!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants